-
Notifications
You must be signed in to change notification settings - Fork 333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Start refactoring character sheet get data #15476
base: master
Are you sure you want to change the base?
Conversation
This looks like it's all thrown in the returned object, making it harder to read. |
Its generally easier to read what data we're returning and its structure when its part of a final return object, even more so after some of the more complicated sub-sections have been moved to private helpers. Significantly better than getting the parent data and casting it to the new return type, and then bolting things on there after the fact and in helper methods and hoping we didn't miss anything. There's also type safety concerns as well. Unfortunately, this isn't complete. There's still that weirdness with how saves and proficiencies are set up. Skills and saves can be lifted up to the creature level, and we should choose between whether to use short (character) or labelShort (NPC) for shortform save labels. We also need to umbrella some of the data, but what umbrellas to use I haven't decided on yet. |
....in short, please read character sheet getData() again (without this change), and then party sheet or deity sheet getData(), and let me know which is actually easier to follow along and get a sense of the structure. I was making an initial step to make the former like the latter, but we can instead make everything like character instead if we prefer it that way. |
While not asserting is nice, giving up separation of individual components of the sheet data isn't. There's a mix of both here, making for a net loss in readability. |
I find that claim pretty tough. I could not follow along at all when I tried to add has-stowing to it. There's no proper trail to follow, and having no type safety was scary on top of that. While there's more work to be done on that front, it definitely went from a 5 to a 20 when the scale is 100, as I can now scroll to the end of getData and usually search where data gets sets. I could further the separations with some more info on where its problematic, perhaps with a more thorough refactor than I was planning for first pass. But let me know if you'd prefer the character sheet's paradigm, because I can convert all newer sheets to that style instead. |
Anyways, I'm going to need something actionable. I can either take this further, or consider the merge workflow we're doing elsewhere a lost cause and revert to this style. #15482 is a trial run converting other things to be like character sheet instead (but it still has the prepareX functions. Character sheet does not split its processing). if the trial run is preferred I can convert everything instead. EDIT: I don't understand what you mean by lack of separating though. Its all a single object either way? |
71b6030
to
d1073bc
Compare
This should make it easier to follow along and also create room to start adding new things.
Some more refactoring needs to be done, but it'll require more intrusive updates to other creature sheets.